feat(mcp): bring-your-own MCP servers for agent sessions - #1892
Merged
Conversation
Phase 1 of idea 01M0QDASJCK3YWVX1GETZTSFWZ — bring-your-own MCP endpoints. Task file rides on the feature branch because main is branch-protected.
Adds the mcp_connections table (migration 0120), shared types, CRUD and resolution services, and widens the vm-agent MCP plumbing from one server to N. - URL is encrypted alongside the token: providers such as Composio issue pre-signed MCP URLs with the credential in the path/query, so the URL is a secret. urlHost (scheme+host) is the display value the API returns instead. - Resolution runs on the agent-session start path and degrades per row: one undecryptable connection must not brick session start (rules 41, 50). - buildSessionMcpServers is the single composition point, called by the shared bootstrap (covers VM + cf-container, rule 61) and the manual workspace route. - Trial path pinned to sam-mcp only and commented: it runs as the anonymous sentinel user and must never resolve connections by that identity. - Contract addition is additive (optional name), so an old vm-agent ignores it and falls back to positional naming (rule 54).
…ions Adds an optional Name to McpServerEntry so N injected servers are distinguishable to the agent (zapier__create_post, not sam-mcp-1__...). - ResolveMcpServerNames is now the single source of truth for naming. The rule previously existed three times (buildAcpMcpServers, codexMcpServerName, and inline in generateVibeConfig) and had already drifted: Vibe emitted sam-mcp-0 for a single server where the others emitted sam-mcp. Unnamed entries keep the exact legacy positional behaviour, and codexMcpTokenEnvVar keeps the historical SAM_MCP_TOKEN / SAM_MCP_TOKEN_<n> forms. - Codex startup required a bearer token for EVERY injected server, which would have made one no-auth user connection break every Codex session. The precondition is now scoped to the reserved sam-mcp entry; URL and CR/LF checks still apply to all servers. - Name is copied through all three field-by-field conversions (normalize, acp->persistence, persistence->acp) plus additive migrateV12. Tests: the round-trip test was verified to fail when any one of the three conversions drops the field, and the Codex pair was verified discriminating in both directions (old unscoped rule fails the relaxation test; deleting the check fails the fail-closed controls).
Routes at /api/mcp-connections (personal) and /api/projects/:projectId/mcp-connections (project), sharing one handler set so the two scopes cannot drift. Project writes require secret:write, so maintainer (which has secret:read but not secret:write) cannot store a credential every member's agents would then use. Tests run against a real in-memory SQLite engine rather than a .where()-ignoring mock, since every scoping guard here IS a SQL predicate (rule 28). Verified discriminating: deleting the scope predicate reddens exactly the three cross-scope attack tests while the owner-path controls stay green, and removing the per-row try/catch reddens exactly the four fault-isolation tests. The vertical slice runs a real HTTP JSON-RPC MCP server (initialize / tools/list / tools/call, bearer auth) and drives the full resolve -> decrypt -> compose path against it, proving the credential SAM injects actually authorizes. Third- party creds are unavailable in CI; this closes the same loop without them.
One McpServersManager serves both scopes (rules 24, 59). Personal scope gets its own Settings tab; project scope sits in the existing Runtime tab beside env vars and files, since it is the same class of thing — configuration injected into every agent session in that project — rather than adding another nav item. Named 'MCP Servers', not 'Connections': that word already means composable-credential connections in SAM (Settings -> Connections manages LLM and cloud provider credentials), and MCP server is the vocabulary users know from Claude Code, Codex and Cursor. Uses TanStack Query with identity-scoped keys, and gates the spinner on 'no data yet' rather than 'refetch in flight' (rules 48, 60). Playwright audit fixes found by actually opening the screenshots (rule 62): - the first-run onboarding modal covered the page, so every capture would have been of the modal while still reporting 'visible'; now suppressed AND asserted absent so the suppression cannot silently regress - duplicate h2/h3 'MCP servers' headings (caught as a strict-mode violation) - 'bearer token' wrapped mid-word because break-all leaked from the host onto the auth label; host and label are now separate spans - the error state used a non-existent 'text-error' class and showed a bare raw message; now the shared Alert with context Note for future audits: Playwright's outputDir IS the screenshot directory, so running one project wipes the other's captures — run both in one invocation.
… shape Adds the public docs page (provider recommendations, scopes, the security note that third-party tools are a prompt-injection surface, and the LinkedIn/Medium reality checks) and the CLAUDE.md changelog entry. Updates the five existing suites that asserted the single-object mcpServer shape. One of them surfaced a real bug rather than just a shape change: the instant-session mocks return a non-array from the query, and decryptAndMerge was called OUTSIDE the try/catch, so a non-array result threw straight through the fault isolation and would have broken session start. The query, the shape check and the merge are now all inside the guard. Full suite: api 8070/8070, web 3449/3449, 0 collection errors in either; lint, typecheck, build, check:fast, migration-safety, do-migration-safety, wrangler-bindings and go vet all green.
…laim The MCP server name rule is implemented twice — MCP_CONNECTION_NAME_PATTERN in TypeScript (rejects on write) and sanitizeMcpServerName in Go (falls back to positional naming). Nothing tied them together, so a drift would silently rename a user's server to sam-mcp-<i> with no error surfaced anywhere. Both now consume one serialized fixture (rule 23). Verified discriminating: letting the Go side accept underscores reddens the contract test immediately. Also corrected the docs, which claimed MCP servers reach 'every supported agent — Claude Code, Codex, Amp, Vibe and OpenCode'. OpenCode gets no MCP config file and only sees servers if its ACP implementation honours the handshake field, which I have not verified. Replaced with what the code actually does, per rule 01.
Ten reviewers ran. Two HIGH and several MEDIUM findings were real bugs, not style. SECURITY - The vm-agent's URL validation error embedded the plaintext URL via %q. That error propagates into tasks.error_message / agent_sessions.error_message — plaintext columns any project member with task:read (including VIEWERS) can read. Since the URL is a secret here, this leaked a credential across a privilege boundary. The message now names the index only. - The same finding exposed a second, independent validator divergence: TS accepted 'HTTPS://host/mcp' (WHATWG lowercases the scheme for its check) while Go's prefix match is case-sensitive, and TS accepted loopback without a port while Go required one. Either shape saved cleanly and then failed EVERY session start for that scope. URLs are now stored WHATWG-normalized, loopback requires an explicit port, and both rules are pinned by the shared contract fixture. - ResolveMcpServerNames honoured the reserved 'sam-mcp' name from any index, so an entry claiming it would take the namespace and silently rename SAM's own endpoint to sam-mcp-1. The vm-agent now reserves it for index 0 itself rather than trusting an upstream convention (rule 51). TESTS THAT COULD NOT SEE THE FEATURE - Review proved by mutation that dropping every bring-your-own entry in the bootstrap left all 124 MCP tests green: the slice tested buildSessionMcpServers directly, and every test that drove the real bootstrap mocked drizzle so resolution always degraded to []. mcp-connection-bootstrap-wiring.test.ts now drives the real startSamAwareAgentSession against a real SQLite engine holding a real encrypted row, and was verified to fail against that exact mutation. - The route layer had zero tests (my own T7, unmet). mcp-connections.test.ts now exercises the REAL project-auth against real membership rows — the sibling suites mock it wholesale, which cannot catch a swapped capability. Verified discriminating: swapping secret:write for project:read reddens exactly the four write cases while the read cases and owner controls stay green. CORRECTNESS / PERF - Drizzle declared FULL unique indexes where the migration creates PARTIAL ones; that difference would wrongly forbid a personal and a project connection sharing a name. - Decrypts now run concurrently and the read is bounded; up to ~100 sequential AES-GCM ops sat on the Instant runtime's start path (rule 43 timeout history). UI - text-fg / border-border / bg-bg / bg-bg-subtle are not in the theme and compiled to nothing — the screenshots only looked right by inheritance. Now real tokens, plus Input/Select/StatusBadge from the design system and the shared ConfirmDialog instead of window.confirm. DOCS - Corrected the agent-support claim, documented the loopback port requirement and the hyphen rule, added the SSRF/DNS-rebinding residual risk, and added the three new env vars to .env.example and configuration.md. api 8093/8093, web 3450/3450, shared 594/594, 0 collection errors; go test, lint, typecheck, Playwright 28/28 all green.
…VR10XMKB679) The one review finding not fixed in this PR gets a tracked idea, a code comment adjacent to the code, and a user-facing caveat — rather than a silent deferral (rule 42). buildAmpMcpServer passes the endpoint URL as a positional CLI arg, so it is readable via /proc by anything in the container. The token is already kept out of argv for exactly that reason. Fixing it needs verification of how mcp-remote accepts a URL from the environment, and shipping an unverified config mechanism is what rule 30 forbids — so it is documented instead of guessed at.
Contributor
The trial-orchestrator-agent-boot test uses a mock MCP callback token (mcp_tok_fixture_abc123) that triggers the generic-api-key rule. This is synthetic test material, not a real secret. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ships MCP Servers — users connect any third-party MCP endpoint (Zapier, executor.sh, Composio, Klavis, or an official GitHub/Notion/Linear/Stripe endpoint) and SAM injects it into every agent session alongside its own
sam-mcp. SAM builds no per-service connectors: the user does the OAuth in their provider's dashboard, and SAM stores onlyurl + optional bearer token.Phase 1 of idea
01M0QDASJCK3YWVX1GETZTSFWZ.Named "MCP Servers", not "Connections" — the idea proposed "Connections", but that word already means composable-credential connections in SAM (
SettingsConnections.tsxmanages LLM + cloud provider credentials). "MCP server" is also the vocabulary users know from Claude Code, Codex and Cursor.Critical implementation notes
The URL is a secret, not just the token. Composio and others issue pre-signed MCP URLs with the credential in the path/query, so
encrypted_urlis AES-256-GCM encrypted exactly like the token and is never returned by any read path.url_host(scheme + host only) is the display value.Rule 61 — every producer of
mcpServersenumerated:agent-session-bootstrap.ts:271(create)buildSessionMcpServersagent-session-bootstrap.ts:329(start)routes/workspaces/agent-sessions.ts(manual workspace session)buildSessionMcpServerstrial-orchestrator/steps.ts(anonymous trial)Producers 1–2 are the shared bootstrap, which serves both the VM (TaskRunner) and cf-container (Instant) runtimes — so one implementation covers both.
Rule 54 — rollout is additive.
McpServerEntry.nameis optional; a vm-agent built before this field ignores it and falls back to its legacy positional naming, producing byte-identical config.VM_AGENT_REQUIRED_VERSIONis generated from the deploy SHA, so new sessions only land on new agents anyway.Pre-existing bugs fixed along the way
session_host_startup.go). One no-auth connection (Composio pre-signed URL) would have broken every Codex session for that user. Now scoped to the reservedsam-mcpentry; URL and CR/LF checks still apply to all servers.buildAcpMcpServers,codexMcpServerName, and an inline literal ingenerateVibeConfig— the last named a lone serversam-mcp-0where the other two saidsam-mcp. Consolidated intoResolveMcpServerNames.Validation
pnpm lintpnpm typecheckpnpm test— api 8093/8093, web 3450/3450, shared 594/594, 0 collection errors (reconciled per rule 02: totals rose from the 8070/3449 baseline by exactly the tests added)go build ./... && go vet ./... && go test ./...(25 packages),pnpm build,pnpm check:fast,pnpm quality:migration-safety,pnpm quality:do-migration-safety,pnpm quality:wrangler-bindingsGuards proven discriminating (mutation-tested, then restored):
requireScopedConnectionsecret:write→project:readon routesNamedropped in any of 3 Go conversions_Staging Verification (REQUIRED)
32651936308, conclusionsuccess. Migration0120_mcp_connections.sqlconfirmed applied via D1;mcp_connectionstable present with all 13 columns.app.sammy.party, authenticated viaPOST api.sammy.party/api/auth/token-loginStaging Verification Evidence
CRUD + security invariants — 16/16 against the live API and UI:
Infrastructure verification (rule 6b). Staging had zero nodes before deploy (rule 27 precondition satisfied), so the test node downloaded the new binary. Provisioned node
01M0QRNAWVBGB0EDTAS2TQCBA7reportedagent_version: 1d0e94ce2710e76b4e6091f72e9474ecebfd0713— this branch's HEAD — confirming both heartbeat and that the new agent build was actually running.Injection proof — discriminating, from the real vm-agent logs:
01M0QS2A28...01M0QS4Y4J...01M0QS575G...count: 2issam-mcp+ the user's connection. The 2→1 drop when the connection is disabled is what makes this discriminating rather than a coincidence — and it independently exercises the second producer. Grepped the full log payload: neither the secret URL path nor the bearer token appears anywhere.Cleanup: node deleted, test connection deleted, verified
0active nodes and0rows remaining. (Hetzner capacity is shared with production — 10 servers.)Screenshots:
staging-mcp-settings-{desktop,mobile}.png,staging-mcp-project-runtime-{desktop,mobile}.png.What was NOT verified on staging, and why
An agent successfully calling a tool on a live third-party endpoint was not verified — that needs a real provider credential, which is not available. It is covered locally:
mcp-connection-injection.test.tsruns a real HTTP JSON-RPC MCP server (initialize/tools/list/tools/call, bearer auth) and drives the full resolve → decrypt → compose path against it, proving the credential SAM injects actually authorizes. Combined with the staging injection evidence above, the only unproven link is the agent's own MCP client, which is not SAM code.UI Compliance Checklist
aria-labelon delete,role="alert"error state, focus-trappingConfirmDialoginstead ofwindow.confirmInput,Select,Button,Alert,Spinner,StatusBadge,ConfirmDialog.codex/tmp/playwright-screenshots/Review caught that the first cut used
text-fg,border-border,bg-bg,bg-bg-subtle— none of which exist in the theme. They compiled to zero rules; the screenshots only looked correct through inherited colors. Fixed to real tokens.Note for future audits: Playwright's
outputDiris the screenshot directory, so running one project wipes the other's captures — run both in a single invocation.End-to-End Verification
Data Flow Trace
Untested Gaps
Post-Mortem
N/A: not a bug fix.This is a feature PR. It does fix two pre-existing latent bugs (Codex tokenless hard-fail; three drifted naming implementations) which are described under "Pre-existing bugs fixed along the way" — neither had shipped a user-visible failure, because both were unreachable until N>1 MCP servers existed.Specialist Review Evidence
e3224c631: (a) the vm-agent's URL validation error embedded the plaintext URL via%q, and that error lands intasks.error_message— a plaintext column any member withtask:read, including viewers, can read; (b) TS/Go validator divergence (case-sensitivity + loopback port) made a saveable URL break every session start. 1 HIGH deferred with justification (see Exceptions). MEDIUM Vibe-cleartext and SSRF documented in the guide.mcp-connection-bootstrap-wiring.test.ts, verified to fail on that exact mutation.ResolveMcpServerNameshonoured the reservedsam-mcpname from any index, letting a third party occupy SAM's trusted namespace. Now enforced at index 0 by the vm-agent itself (rule 51).window.confirm, hand-rolled chip. All four fixed.buildSamMcpEntryextracted (last duplicatedhttps://api.${BASE_DOMAIN}/mcpliteral); inaccurate docstring corrected; cross-reference to the sibling runtime-assets system added..env.exampleandconfiguration.md; annotated the superseded backlog task.Exceptions
Scope: The Amp harness passes the MCP endpoint URL as a positional CLI argument (
buildAmpMcpServer), so it is readable via/procinside the workspace. The bearer token is already kept out of argv for exactly this reason.Rationale: Fixing it requires knowing how
mcp-remoteaccepts a URL from the environment. I could not verify that without running it, and rule 30 forbids shipping an unverified config mechanism. Marginal exposure: that agent already holds the URL and the user has shell access in their own workspace. Tracked as idea01M0QQ7PTBDPG0DVR10XMKB679, with a comment at the call site and a caveat in the public docs (rule 42 — not a silent deferral).Expiration: Before Amp is recommended for pre-signed-URL providers.
Scope:
apps/api/src/services/node-agent.tsis 908 lines, over rule 18's 800-line hard limit.Rationale: It was already 888 lines before this PR; this change adds ~20. Splitting it touches many importers and would make this diff materially harder to review. Tracked in the same idea.
Expiration: Next change to that file.
Agent Preflight (Required)
Classification
External References
Official documentation consulted for the provider landscape and the MCP endpoint shape: Model Context Protocol, Zapier MCP, executor.sh, Composio hosted MCP platforms, Klavis, Codex remote MCP config, Claude Code MCP. Provider/ToS reality checks (LinkedIn API limits, Medium API closure) are cited in idea
01M0QDASJCK3YWVX1GETZTSFWZ.Codebase Impact Analysis
packages/shared—src/types/mcp-connection.ts(new),src/vm-agent-contract.ts(additivename),src/constants/defaults.ts,src/fixtures/mcp-server-name-contract.json(new)apps/api—src/db/schema.ts,src/db/migrations/0120_mcp_connections.sql(new),src/services/mcp-connections.ts+mcp-connection-resolution.ts(new),src/services/node-agent.ts,src/services/agent-session-bootstrap.ts,src/routes/mcp-connections.ts(new),src/routes/workspaces/agent-sessions.ts,src/durable-objects/trial-orchestrator/steps.ts,src/services/limits.ts,src/env.ts,src/schemas/packages/vm-agent—internal/acp/mcp_server_names.go(new),internal/acp/gateway.go,internal/acp/session_host.go,internal/acp/session_host_startup.go,internal/persistence/store.go,internal/server/workspaces.go,internal/server/agent_ws.goapps/web—src/components/mcp-servers/,src/pages/SettingsMcpServers.tsx,src/pages/Settings.tsx,src/pages/ProjectSettings.tsx,src/App.tsx,src/lib/api/mcp-connections.ts,src/lib/query-options/mcp-connections.tsapps/www—src/content/docs/docs/guides/mcp-servers.md(new),src/content/docs/docs/reference/configuration.md,astro.config.tsDocumentation & Specs
apps/www/src/content/docs/docs/guides/mcp-servers.md(new public guide, registered in the sidebar)apps/www/src/content/docs/docs/reference/configuration.md(three new env vars)apps/api/.env.exampleCLAUDE.md(Recent Changes:byo-mcp-servers)tasks/archive/2026-08-23-byo-mcp-servers.md;tasks/backlog/2026-02-15-user-configurable-mcp-servers.mdannotated as supersededConstitution & Risk Check
Principle XI (No Hardcoded Values): three new limits ship as
DEFAULT_*constants with env overrides, threaded to their enforcement points. All URLs derive fromBASE_DOMAINvia the singlebuildSamMcpEntry. Name-length is a named constant, and the TS/Go copies are pinned together by a shared fixture.Key risks and how they are handled:
sam-mcp" so an agent always starts (rules 41, 50).